Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Fix compatibility with recent versions of CocoaPods (> 1.5.3) #67

Merged
jtreanor merged 2 commits into from
Mar 29, 2019
Merged

Fix compatibility with recent versions of CocoaPods (> 1.5.3) #67

jtreanor merged 2 commits into from
Mar 29, 2019

Conversation

@jtreanor
Copy link
Contributor

@jtreanor jtreanor commented Mar 27, 2019 •
edited
Loading

This change fixes compatibility with recent versions of CocoaPods. Once merged it will be possible to update CocoaPods in all of our projects.

The issue we are seeing is caused by GoogleSignInRepacked. It seems that it is not behaving as a true dynamic framework and some linking changes by CocoaPods mean it doesn't work anymore. The issue is described here.

The solution is to use the official GoogleSignIn pod and add s.static_framework = true to the podspec.

Changing to a static framework means that everything gets added to the main bundle. This can lead to conflicts once it is installed in an app. I have made a few changes to mitigate this:

  • The podspec now uses resource_bundles to create WordPressAuthenticatorResources.bundle which contains the .xcassets file and the json animation files. Storyboards and xibs are still loaded from the main bundle.
  • This meant that usages of Bundle(for: WordPressAuthenticator.self) needed to be updated for this new bundle.
  • The storyboards also needed to be updated to not look for a module named WordPress for classes. This was likely left over from extracting the storyboards from WordPress-iOS.

To test:

Since the risk here is that resources won't be correctly loaded, its probably best to test this from within WordPress:

  1. Check out this branch. Check out develop of WordPress-iOS.
  2. Uncomment the line in your WordPress-iOS Podfile that says pod 'WordPressAuthenticator', :path => '../WordPressAuthenticator-iOS' and comment the line above.
  3. Run rake dependencies
  4. Build and run the app.
  5. Run through a login flow and make sure nothing looks out of place (missing icons, animations etc.).

@jtreanor jtreanor requested a review from nheagy March 27, 2019 16:18
jtreanor
jtreanor commented Mar 27, 2019
View reviewed changes
WordPressAuthenticator.podspec

s.dependency 'Gridicons', '~> 0.15'
s.dependency 'GoogleSignInRepacked', '4.1.2'
s.dependency 'GoogleSignIn', '4.1.2'
Copy link
Contributor Author

@jtreanor jtreanor Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably update this too since this is an old version but better to do one thing at a time 😄

@jtreanor
Copy link
Contributor Author

jtreanor commented Mar 28, 2019 •
edited
Loading

It's worth pointing out what the alternative solutions would be here:

  1. Fix the issue in CocoaPods. While we could certainly do this and see if it would be accepted. The new behavior is arguably correct since GoogleSignInRepacked doesn't behave as a true dynamic framework.
  2. Fix GoogleSignInRepacked. We could take another look at how we repackage GoogleSignIn. There is another project that does this and it works with CP 1.6.0: https://github.com/crspybits/SMGoogleSignIn. We could use a similar approach. I think it's better to get rid of the repackaging step altogether though.

@astralbodies
Copy link
Contributor

astralbodies commented Mar 29, 2019

I'd also test to make sure packaging a release works properly and passes the App Store tests. I remember something vaguely when @jleandroperez was working on this solution regarding that.

@jleandroperez
Copy link
Contributor

jleandroperez commented Mar 29, 2019

I can't recall exactly in which spot things literally exploded (and forced us to repack as a dynamic framework).

Archiving a binary (and doing a test upload to the AppStore), along with running the development binary directly in the device should suffice. As far as i remember the issue itself was a CocoaPods limitation. Since the Repacked framework was built... Mayb 2018?, perhaps this CocoaPods issue is already solved.

Please feel free to invoke me anytime, i'd be more than happy to help!!! YAY Opensource!!

@jtreanor
Copy link
Contributor Author

jtreanor commented Mar 29, 2019

Good suggestion @astralbodies. I'll try that.

Thanks for the insight @jleandroperez! The CocoaPods limitation that transitive dependencies can't be static still exists, but the newish s.static_framework = true feature seems to allow this.

I'll dig and update here with what I find out.

@jleandroperez
Copy link
Contributor

jleandroperez commented Mar 29, 2019

Awesome!! Getting rid of the repacked binary would definitely be a good move. Too much blood went into that repack script!!!

@jtreanor
Copy link
Contributor Author

jtreanor commented Mar 29, 2019

Ok, so I have built an AppStore archive of WPiOS that points to this branch and used "Validate App" from Organiser. It passed all checks:

image

I have also tested a debug build WPiOS on a physical device, including doing a full login flow and everything looks good.

nheagy
nheagy approved these changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@nheagy nheagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to login with Google with this branch. Seems good!

:shipit:

@jtreanor
Copy link
Contributor Author

jtreanor commented Mar 29, 2019

Thanks @nheagy!

@jtreanor jtreanor merged commit d665345 into develop Mar 29, 2019
@jtreanor jtreanor deleted the fix-cocoapods branch March 29, 2019 16:46
@jtreanor jtreanor mentioned this pull request Mar 29, 2019
Merged
@jleandroperez
Copy link
Contributor

jleandroperez commented Mar 29, 2019

@jtreanor I think pod install would throw the error right away. Glad to see this fix go thru, thanks everyone!!

:hug:

This was referenced Apr 1, 2019
Merged
Merged
@mindgraffiti mindgraffiti mentioned this pull request Apr 4, 2019
Merged
1 task
This was referenced Apr 5, 2019
Merged
Closed
Merged
Closed
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@nheagy nheagy nheagy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtreanor @astralbodies @jleandroperez @nheagy